-
Notifications
You must be signed in to change notification settings - Fork 131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Benchmarking tool using cost estimator #235
Conversation
1edf769
to
25238ea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey! Thanks for the PR. A couple of remarks:
- I'd just like to have it within a
cost-estimator
feature flag. - I think is a bit unreasonable to be forced to run the
MSM
s to then get the cost. This tool is an estimator, hence it should estimate. Not benchmark directly.
I think a much better way is to log in the CSV properties like:
- Num of Advice cells
- Num of lookup-active columns
- Num of fixed columns
- Maximum rotation
- Num of copy-enabled columns
- Maximum lookup degree
- Maximum expression degree
- Degree of the circuit (lenght)
- Proof Size
Note that most of these things can be counted in Commitments
(for both, operations and storage).
Hence, we have a universal unit that will be useful across all possible curves/primitives.
Would like to hear more from @davidnevadoc @han0110 @kilic
halo2_proofs/src/dev/cost_model.rs
Outdated
CommitmentScheme::KZG => { | ||
// Polycommit KZG: | ||
// - quotient polynomial commitment (COMM bytes) | ||
size(1, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For GWC19 it'd be size(set(rotations).len(), 0)
and without the 1
from multiopen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done using a HashSet
, so rotations are not repeated. Is that right? a36267b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @han0110
I think the same with these 2 bullets. It would be beyond this porting efford but info of number of gates and gate with their add, mul complexity might be usuful to see since it adds up to evaluation time |
|
||
/// Options to build a circuit specifiction to measure the cost model of. | ||
#[derive(Debug)] | ||
pub struct CostOptions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should include ::shuffle
arguments as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a pointer to docs of how the shuffle argument works? Looking at the code it seems that it only uses two rotations (\omega X
and \omega
), is that right? 67440c9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the feature is defined in the README (not sure if it points to any extra docs). IIRC the cost is to commit to the shuffled column which means extra commitment.
Unsure about the rotations though. ping @kilic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
er to docs of how the shuffle argument works? Looking at the code it seems that it only uses two rotations (
\omega X
and\omega
), is that right? 67440c9
@iquerejeta Here it goes #185. It's kind of simplified version of lookup argument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks. And I think the computation of the lookup queries was wrong. Fixed it here
c1184bb
to
8464681
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for addressing all the comments!
419f77c
to
3d789b8
Compare
Co-Authored-By: Iñigo Querejeta Azurmendi <[email protected]>
Use built-in function to compute cs max_degree
) * Benchmarking tool using cost estimator Co-Authored-By: Iñigo Querejeta Azurmendi <[email protected]> * Address review comments * Add example of cost-estimator usage * Cost model example behind feature * Make report generic over field size and commitment scheme * Nits * Use serde for printing the report * Calculate max expression degree Use built-in function to compute cs max_degree * Add cost computation for KZG GWC * Add shuffle argument * Nits * Remove CLI * Remove FromStr impl - not needed without CLI * Fix queries of Lookup * There is only one permutation argument * Remove dependency of halo2_gadgets for cost-estimator example --------- Co-authored-by: Henry Blanchette <[email protected]>
This PR moves the functionality used in
halo2_proofs/examples/cost-model.rs
into a new filehalo2_proofs/src/dev/cost_model.rs
, which is exposed publicly. We expose two additional functions,from_circuit_to_model_circuit
andfrom_circuit_to_cost_model_options
to get the benchmarks for a given Plonk circuit.